Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

expose preventEviction as unsafePreventEviction for Durable Objects #726

Merged
merged 6 commits into from
Oct 27, 2023

Conversation

RamIdeas
Copy link
Contributor

No description provided.

for durable object bindings
@changeset-bot
Copy link

changeset-bot bot commented Oct 26, 2023

⚠️ No Changeset found

Latest commit: 44b1223

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@RamIdeas RamIdeas changed the base branch from master to tre October 26, 2023 15:21
@RamIdeas RamIdeas closed this Oct 26, 2023
@RamIdeas RamIdeas reopened this Oct 26, 2023
Copy link
Contributor

@penalosa penalosa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be nice to see a test here that:

  • Didn't set this flag, waited for 10 seconds, and verified that the DO was evicted
  • Did set this flag, waited for 10 seconds, and verified that the DO wasn't evicted

@RamIdeas
Copy link
Contributor Author

@penalosa I've added the tests but... as much as I agree with the plea for tests, I'm not sure the 20+ seconds added to all test runs from here on out is worth it

@mrbbot what do you think?

Comment on lines 284 to 291
if (existingInfo?.unsafeUniqueKey !== unsafeUniqueKey) {
throw new MiniflareCoreError(
"ERR_DIFFERENT_UNIQUE_KEYS",
`Multiple unsafe unique keys defined for Durable Object "${className}" in "${serviceName}": ${JSON.stringify(
unsafeUniqueKey
)} and ${JSON.stringify(existingUnsafeUniqueKey)}`
)} and ${JSON.stringify(existingInfo?.unsafeUniqueKey)}`
);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you add a similar check for unsafePreventEviction? Since a Durable Object can have multiple bindings to it, and this configuration is defined at the binding level, but set in workerd with each Durable Object, we need to make sure unsafePreventEviction is configured the same on all bindings to the same Durable Object.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added. Do you want a test for this behaviour? Can't see one for existingUnsafeUniqueKey

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not too worried about it, but if you're offering... 😃

packages/miniflare/src/plugins/shared/index.ts Outdated Show resolved Hide resolved
packages/miniflare/test/plugins/do/index.spec.ts Outdated Show resolved Hide resolved
@mrbbot
Copy link
Contributor

mrbbot commented Oct 27, 2023

Going to merge this now. I'm about to start the migration to workers-sdk, and want to avoid having to recreate this PR. We can add additional tests later. 👍

@mrbbot mrbbot merged commit b634851 into tre Oct 27, 2023
9 checks passed
@mrbbot mrbbot deleted the do-prevent-eviction branch October 27, 2023 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants